Skip to content

prefer-string-raw: Forbid unnecessary String.raw #2695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

som-sm
Copy link
Contributor

@som-sm som-sm commented Jun 25, 2025

Closes #2652

This PR simply loops over all the quasis and checks if any of them contain a backslash. If none of them do, then the rule starts complaining.

Also, the suggestion is a template string(`) only if there are expressions or the string contains line breaks, otherwise, the suggestion is always a single quotes string (').

yield fixer.replaceText(node, suggestion);
},
};
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need check this.

`\u0040`
'@'
String.raw`\u0040`
'\\u0040'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check .cooked === .raw?

Copy link
Contributor Author

@som-sm som-sm Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't get this.


The following tests already work:

valid: [
  'a = String.raw`\\u0040`'
]
invalid: [
  'a = String.raw`\u0040`'
]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I thought we suggest `\u0040` over String.raw`\u0040`, I must made a mistake.

Anyway, do you think we can check .cooked === .raw instead of checking contains \?

Copy link
Contributor Author

@som-sm som-sm Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, do you think we can check .cooked === .raw instead of checking contains \?

Yeah, we can, updated in 2eb20bf.

Copy link
Contributor Author

@som-sm som-sm Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above update, surfaced an interesting issue.

So, after this update, I'm getting a parsing error in the puppeteer codebase at the following line:
https://github.com/puppeteer/puppeteer/blob/832831d21c42ff542300c194c2b4f07274d8f82f/test/src/queryhandler.spec.ts#L373


Here's a simple reproduction of the same:

Source code:

let a = "\\38";

This correctly gets fixed to:

let a = String.raw`\38`;

However, the above String.raw`\38` expression fails with the typescript parser, throwing the following error:

Parsing error: Octal escape sequences are not allowed. Use the syntax '\x03'.

The includes(BACKSLASH) check wasn’t failing because it allowed the function to exit early when a backslash was detected in the raw value.

But, with the .cooked === .raw check, it fails because both cooked and raw values here are the same ('\\38'), causing the check to pass.
cooked value can't be '\38' because that '\38' is syntactically incorrect, maybe because of this the parser keeps the cooked value same as raw.

value: { cooked: '\\38', raw: '\\38' }

This works fine with the babel parser because looks like it sets the cooked value to null in cases like these.

value: { raw: '\\38', cooked: null }

So, I think it's better to keep the includes(BACKSLASH) check because it operates on the raw value, which seems to be a better choice in this case.

LMK if this makes sense, I'll add the let a = String.raw`\38`; test with the typescript parser.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bug in typescript-eslint, can you report to them? I can do it if you don't want to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate if you could, I'm fairly new to parsers, so I might not be able to explain it clearly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fisker fisker changed the title prefer-string-raw: Remove unnecessary usages prefer-string-raw: Forbid unnecessary String.raw Jun 26, 2025
@som-sm som-sm requested a review from fisker June 26, 2025 17:24
outdent`
a
String.raw\`a\${b}\`
`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for

function foo() {
	return String.raw
			// Comment
			``
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments like these weren't being preserved, had to update the implementation a bit, refer cc0a128.

Copy link
Collaborator

@fisker fisker Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is not the point, in this case, we can't simply remove String.raw need add parenthesis after return.

Copy link
Collaborator

@fisker fisker Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it should fix to

function foo() {
	return (
			// Comment
			``)
}

There are similar cases exists, please search other rules.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search addParenthesizesToReturnOrThrowExpression

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, apologies for the confusion.

@som-sm som-sm force-pushed the feat/remove-unnecessary-string-raw-usage branch from 0c30a3c to ffcc738 Compare June 27, 2025 14:37
@som-sm som-sm requested a review from fisker June 27, 2025 14:52
}

yield fixer.replaceText(node.quasi, suggestion);
yield fixer.remove(node.tag);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect, since tag can be parenthesis.

((String.raw))``

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use removeParentheses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: opposite of prefer-string-raw
2 participants